Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Message Signature to identify message sender and sending messages back #130

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Message Signature to identify message sender and sending messages back #130

merged 1 commit into from
Dec 17, 2019

Conversation

onsails
Copy link
Member

@onsails onsails commented Dec 13, 2019 •
edited
Loading

Resolves #119

@onsails onsails requested review from vertexclique and r3v2d0g and removed request for vertexclique December 13, 2019 23:22
@onsails onsails force-pushed the sender branch 5 times, most recently from 811c7d0 to d6c3d28 Compare December 15, 2019 14:30
vertexclique
vertexclique requested changes Dec 15, 2019
View reviewed changes
Copy link
Member

@vertexclique vertexclique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good, but need a little more work with doctests, examples and overall documentation. Also, we might need to refine the structure of the Signature.

bastion/src/context.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Outdated Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
@onsails onsails mentioned this pull request Dec 15, 2019
Merged
vertexclique
vertexclique approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@vertexclique vertexclique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments will be addressed in dead letters development

r3v2d0g
r3v2d0g suggested changes Dec 16, 2019
View reviewed changes
bastion/src/message.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Outdated Show resolved Hide resolved
bastion/src/context.rs Show resolved Hide resolved
bastion/src/message.rs Outdated Show resolved Hide resolved
bastion/src/context.rs Show resolved Hide resolved
bastion/src/context.rs Outdated Show resolved Hide resolved
bastion/src/context.rs Outdated Show resolved Hide resolved
bastion/src/context.rs Outdated Show resolved Hide resolved
bastion/src/envelope.rs Show resolved Hide resolved
@onsails onsails force-pushed the sender branch 4 times, most recently from 9ac8d38 to 6f1e16c Compare December 16, 2019 21:39
@onsails onsails requested a review from r3v2d0g December 16, 2019 21:47
@onsails onsails force-pushed the sender branch 3 times, most recently from 2e01de5 to 8b61725 Compare December 16, 2019 23:32
vertexclique
vertexclique reviewed Dec 17, 2019
View reviewed changes
Copy link
Member

@vertexclique vertexclique left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the tuple variant issue, this looks good. We should get rid of tuples at the public API. This will be extremely confusing for a newcomer.

That was worker enumeration discard my comment.

bastion/examples/parallel_computation.rs Show resolved Hide resolved
vertexclique
vertexclique approved these changes Dec 17, 2019
View reviewed changes
Copy link
Member

@vertexclique vertexclique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any blocker to merge this one.

r3v2d0g
r3v2d0g suggested changes Dec 17, 2019
View reviewed changes
bastion/src/lib.rs Outdated Show resolved Hide resolved
bastion/src/context.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
bastion/src/path.rs Show resolved Hide resolved
@r3v2d0g
Copy link
Contributor

r3v2d0g commented Dec 17, 2019

Thank you for your big contribution 💯 :)

@onsails onsails merged commit 1f60149 into master Dec 17, 2019
@onsails onsails deleted the sender branch December 17, 2019 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r3v2d0g r3v2d0g r3v2d0g approved these changes

@vertexclique vertexclique vertexclique approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Identify messages sender and send messages back
3 participants
@onsails @r3v2d0g @vertexclique